-
Notifications
You must be signed in to change notification settings - Fork 10
VIDSOL-16: Localization Support (Implement internationalization with react-i18next) #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f838b47 to
ac095a9
Compare
cpettet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arnaud-lebreton-rofim,
Thanks for opening this PR! Right now, the branch is coming from your fork, which prevents parts of our CI from running (integration tests, some security scans). Could you please recreate this PR from a branch on this repository instead of your fork? That way, our checks will run automatically.
Also, the PR looks good, just left a few nits but nothing is blocking aside from the CI. Thanks again for contributing!
| const { forceMute } = useSessionContext(); | ||
| const [isModalOpen, setIsModalOpen] = useState<boolean>(false); | ||
|
|
||
| const muteParticipantText: DialogTexts = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, could we revert the rename here?
| contents: `Mute ${stream?.name} for everyone in the call? Only ${stream?.name} can unmute themselves.`, | ||
| primaryActionText: 'Mute', | ||
| secondaryActionText: 'Cancel', | ||
| const participants: MutingDialogTexts = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, could we revert the variable name change?
frontend/src/locales/en.json
Outdated
| "screenSharing.dialog.title": "Do you want to share your screen?", | ||
| "screenSharing.title.start": "Start screen share", | ||
| "screenSharing.title.stop": "Stop screen share", | ||
| "screenSharing.tooltip.ariaLabel": "add", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. Since we're in the area, could we change this aria-label to be more descriptive? Maybe "Start or stop screen share" or similar.
| "screenSharing.tooltip.ariaLabel": "add", | |
| "screenSharing.tooltip.ariaLabel": "Start or stop screen share", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent refactoring! 🎉
ac095a9 to
60504d7
Compare
|
hi @cpettet, Currently i cannot push a branch in this repo. |
cpettet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Great job! ![]()
|
Hi @arnaud-lebreton-rofim , Thanks! |
behei-vonage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
|
Hi @cpettet, |
|
Hi @arnaud-lebreton-rofim , |
7922c84
60504d7 to
7922c84
Compare
7922c84 to
74f5b76
Compare
cpettet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Great job! ![]()
|
Tested LGTM!! Also checked the integration tests, Looks good!! 🚀 |
What is this PR doing?
Hi there,
We at Rofim, have been using Vonage as video meeting solution in our application for some time.
Initially, we implemented our own service with the @vonage/video-express library.
Recently, we forked this vonage-video-react-app project and, with a few tweaks, were able to customize it to meet our needs.
However, there’s one key feature we believe would make the forking process (such as pulling future updates) much smoother: internationalization.
As a multilingual platform, we currently provide our users with an interface in six languages, so we also need to localize our fork of Vonage accordingly.
That’s why we’re excited to submit this PR, which introduces react-i18next for translations.
The main changes include:
Replacing hard-coded strings with translation keys.
Moving certain constants into translation files.
we reworked the date-service to leverage JavaScript’s built-in locale formatting:
Mon → lun
9 PM → 21:00
Adding new languages easily with config files
We kept the existing test suite intact to ensure no regressions.
We’re really happy to share this PR with you, and we’d be glad to hear any feedback or suggestions you may have.
How should this be manually tested?
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDSOL-16
Checklist
[x] Branch is based on
develop(notmain).[ ] Resolves a
Known Issue.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md?[ ] Resolves an item reported in
Issues.If yes, which issue? Issue Number?